Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding function ctd_plot and solving conflicts #115

Merged
merged 24 commits into from
Aug 6, 2024

Conversation

FloraSauerbronn
Copy link
Contributor

Following instructions from #110
New branch from upstream and add new ctd plot function.

This was referenced Jun 28, 2024
@ocefpaf
Copy link
Member

ocefpaf commented Jul 4, 2024

@FloraSauerbronn this is the image diff:

result-failed-diff

Again, it does seem to be related to fonts. Can you try to create a fresh environment with conda just to ensure you will get the same fonts we have in the CIs? We have some instructions on how install miniforge, a conda provider, and create environments in https://ioos.github.io/ioos_code_lab/content/ioos_installation_conda.html

Then, once you have conda, you can create an environment for gliderpy with the commands:

conda create --name GLIDERPY python=3 --file requirements.txt --file requirements-dev.txt
pip install -e . --no-deps --force-reinstall

With that said, if you check the logs, the RMS on the image diff is ~16.5. See https://github.com/ioos/gliderpy/actions/runs/9713713840/job/26811095367?pr=115#step:5:24.

That means you can add a tolerance of 17 for the test and get them to pass.

PS: Also check the pre-commit logs and see if you can sort them out.

@FloraSauerbronn
Copy link
Contributor Author

@ocefpaf But I should change the tolerance even if using the Conda from IOOS work ?

@ocefpaf
Copy link
Member

ocefpaf commented Jul 10, 2024

@ocefpaf But I should change the tolerance even if using the Conda from IOOS work ?

Did you try that on Windows and the diffs are still ~16.5? If so, yes.

FloraSauerbronn and others added 2 commits July 10, 2024 16:24
Adding white spaces and top description.
:return: figure, axes
"""
g = df.groupby(["longitude", "latitude"])
profile = g.get_group((list(g.groups)[0]))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FloraSauerbronn in these two lines line we are:

  1. grouping the data in lon, lat poitns, so each group is a CTD profile;
  2. getting the first group, aka, the first CTD profile.

A user may want to:
a. plot profile x by itself, not necessarily the first
b. plot all profiles for a quick inspection

For a we should introduce a new kw arg were the [0] above would the profile number.
Option b is tricky b/c we can easily blow up the memory if the section is too big.

Can try to implement option a?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I will try to come up with something today. I am still trying to understand why the pre-commit didn't work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean why pre-commit failed or why they did not work on your machine? Either way, don't worry about pre-commits until the last minute when the PR is ready to merge. We can use autofix to correct it then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay.

In the kwargs of the CTD plot, I would like the user to be able to choose which latitude and longitude they want to see a profile for. However, the problem is that they might not get the specific latitude or longitude they want. If the kwargs were just a simple index, I don't know how that would help because the user wouldn't know which latitude and longitude the profile corresponds to by just choosing an index in a dataframe. There needs to be a way for the user to see which latitudes and longitudes are available and then choose accordingly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try something simpler first. A user already selected a bounding box and is aware of the positions where the profiles lies. So, accessing profiles based on integer indices is not a bad approach. You would need to make that [0] something like [idx] and add an idx argument with the accompanying documentation for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, feeling a bit weird about such a simple solution. I keep thinking about adding a title to the plot with the latitude and longitude, but the user can do this on their own.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing the push again with changes..

@register_dataframe_method
def plot_ctd(
df: pd.DataFrame,
idx: int,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename this to something more descriptive. Maybe profile_number?

) -> tuple:
"""Make a CTD profile plot of pressure vs property
depending on what variable was chosen.
:param idx: index of position
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Describe that idx is a profile number.

Suggested change
:param idx: index of position
:param idx: index of position


if ax is None:
fig, ax1 = plt.subplots(figsize=(5, 6))
ax1.plot(profile[var], -profile["pressure"], label=var, color=color)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pressure should be a positive number. We can invert the axis here instead with something like:

    if not ax.yaxis_inverted():
        ax.invert_yaxis()

BTW, I'm confused with ax, ax1, ax2. Aren't we plotting just a single variable on ax?

Suggested change
ax1.plot(profile[var], -profile["pressure"], label=var, color=color)
ax1.plot(profile[var], profile["pressure"], label=var, color=color)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are using the command twiny for duplicating axes. I couldn't solve it just using "ax" for both, so I decided to specify.

@ocefpaf
Copy link
Member

ocefpaf commented Jul 26, 2024

@FloraSauerbronn let's get this one in first before iterating on #122. Things may change there after we merge this one.

Comment on lines 116 to 122
fig, ax1 = plt.subplots(figsize=(5, 6))
ax1.plot(profile[var], profile["pressure"], label=var, color=color)
ax1.set_ylabel("Pressure")
ax1.set_xlabel(var)
ax1.legend()
ax1.invert_yaxis()
return fig, ax1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please correct me if I'm wrong but it is not clear to me why you renamed the ax object here.

Suggested change
fig, ax1 = plt.subplots(figsize=(5, 6))
ax1.plot(profile[var], profile["pressure"], label=var, color=color)
ax1.set_ylabel("Pressure")
ax1.set_xlabel(var)
ax1.legend()
ax1.invert_yaxis()
return fig, ax1
fig, ax = plt.subplots(figsize=(5, 6))
ax1.plot(profile[var], profile["pressure"], label=var, color=color)
ax.set_ylabel("Pressure")
ax.set_xlabel(var)
ax.legend()
ax.invert_yaxis()
return fig, ax

Comment on lines 124 to 127
fig = ax.get_figure()
ax2 = ax.twiny() # Create a new twinned axis
ax2.plot(profile[var], profile["pressure"], label=var, color=color)
ax2.set_xlabel(var)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this? We are supporting a single variable, why plot it twice?

Suggested change
fig = ax.get_figure()
ax2 = ax.twiny() # Create a new twinned axis
ax2.plot(profile[var], profile["pressure"], label=var, color=color)
ax2.set_xlabel(var)

Comment on lines 129 to 132
# Handle legends
lines, labels = ax.get_legend_handles_labels()
lines2, labels2 = ax2.get_legend_handles_labels()
ax.legend(lines + lines2, labels + labels2, loc="lower center")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is a reason for the code above, we need. However, my feeling is that we don't need the twiny axis and this code block can also be removed.

lines2, labels2 = ax2.get_legend_handles_labels()
ax.legend(lines + lines2, labels + labels2, loc="lower center")

return fig, ax2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that you also need to regenerate the figures when you change them. Otherwise you will be comparing a new modified image to the old one.

Suggested change
return fig, ax2
return fig, ax

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're correct . I'm changing it and doing the push again.

@FloraSauerbronn
Copy link
Contributor Author

PR was reviewed. plotting.py and test_plotting.py were altered, and a new baseline image was created for the ax in plot_ctd.

Let me know if you need any more adjustments!

Comment on lines 136 to 138
lines, labels = ax.get_legend_handles_labels()
lines2, labels2 = ax.get_legend_handles_labels()
ax.legend(lines + lines2, labels + labels2, loc="lower center")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this now that we have a single ax.

Suggested change
lines, labels = ax.get_legend_handles_labels()
lines2, labels2 = ax.get_legend_handles_labels()
ax.legend(lines + lines2, labels + labels2, loc="lower center")
Suggested change
lines, labels = ax.get_legend_handles_labels()
lines2, labels2 = ax.get_legend_handles_labels()
ax.legend(lines + lines2, labels + labels2, loc="lower center")

@FloraSauerbronn
Copy link
Contributor Author

The pre-commit stopped running on my computer. I don't know why, so I couldn't review the errors. I will try again later.

@ocefpaf
Copy link
Member

ocefpaf commented Jul 30, 2024

The pre-commit stopped running on my computer. I don't know why, so I couldn't review the errors. I will try again later.

Ignore the pre-commit failures for now. If everything is OK after the review we can use the bot command to fix it and merge.

gliderpy/plotting.py Outdated Show resolved Hide resolved
ax.plot(profile[var], profile["pressure"], label=var, color=color)
ax.set_ylabel("Pressure")
ax.set_xlabel(var)
ax.invert_yaxis()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be an if-check block outside of the ax creation. We had it before if I'm not mistaken.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem occurs when calling the legend while using the twiny axis. We have three scenarios:

1. When using just one axis (not referring to a second one like ax2), the legend will only display the last variable plotted if twiny is used, so we need to create twoo box one for each legend and specify where they should be placed.

`
@register_dataframe_method
def plot_ctd(
df: pd.DataFrame,
profile_number: int,
var: str,
ax: plt.Axes = None,
color: str | None = None,
) -> tuple:
"""Make a CTD profile plot of pressure vs property
depending on what variable was chosen.

:param profile_number: profile number of CTD
:param var: variable to plot against pressure
:param ax: existing axis to plot on (default: None)
:param color: color for the plot line (default: None)
:return: figure, axes
"""
g = df.groupby(["longitude", "latitude"])
profile = g.get_group(list(g.groups)[profile_number])

if ax is None:
    fig, ax = plt.subplots(figsize=(5, 6))
    ax.plot(profile[var], profile["pressure"], label=var, color=color)
    ax.set_ylabel("Pressure")
    ax.set_xlabel(var)
    ax.invert_yaxis()
    ax.legend(loc="lower center", bbox_to_anchor=(0.5, 0.07))
    return fig, ax
else:
    ax = ax.twiny()  # Create a new x-axis on top
    ax.plot(profile[var], profile["pressure"], label=var, color=color)
    ax.set_xlabel(var)
    ax.legend(loc="lower center")
    return ax.figure, ax

fig, ax = plot_ctd(df, 0, var="temperature", color="blue")
fig, ax = plot_ctd(df, 0, var="salinity", ax=ax, color="red")`

2. Or calling the legend inside each clouse (if ax is none and else) without defining each ax they would overlap eachother.

3. Or we defined each ax and call the legens like this so they can be in the same box

`@register_dataframe_method
def plot_ctd(
df: pd.DataFrame,
profile_number: int,
var: str,
ax: plt.Axes = None,
color: str | None = None,
) -> tuple:
"""Make a CTD profile plot of pressure vs property
depending on what variable was chosen.

:param profile_number: profile number of CTD
:param var: variable to plot against pressure
:param ax: existing axis to plot on (default: None)
:param color: color for the plot line (default: None)
:return: figure, axes
"""
g = df.groupby(["longitude", "latitude"])
profile = g.get_group(list(g.groups)[profile_number])

if ax is None:
    fig, ax = plt.subplots(figsize=(5, 6))
    lns1 = ax.plot(profile[var], profile["pressure"], label=var, color=color)
    ax.set_ylabel("Pressure")
    ax.set_xlabel(var)
    ax.invert_yaxis()
    
    # Get handles and labels for the legend
    handles, labels = ax.get_legend_handles_labels()
    ax.legend(handles, labels, loc="best")
    return fig, ax
else:
    ax2 = ax.twiny()  # Create a new x-axis on top
    lns2 = ax2.plot(profile[var], profile["pressure"], label=var, color=color)
    ax2.set_xlabel(var)
    
    # Get handles and labels for both axes
    handles1, labels1 = ax.get_legend_handles_labels()
    handles2, labels2 = ax2.get_legend_handles_labels()
    
    # Combine handles and labels
    handles = handles1 + handles2
    labels = labels1 + labels2
    
    # Set the combined legend
    ax.legend(handles, labels, loc="best")
    
    return ax.figure, ax2

Example usage:

fig, ax = plot_ctd(df, 0, var="temperature", color="blue")
fig, ax = plot_ctd(df, 0, var="salinity", ax=ax, color="red")
`

@ocefpaf ocefpaf merged commit c9df903 into ioos:main Aug 6, 2024
12 checks passed
@ocefpaf
Copy link
Member

ocefpaf commented Aug 6, 2024

🎉

@FloraSauerbronn FloraSauerbronn deleted the ctd_plots branch August 30, 2024 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants